Skip to content

Feature/usblogupload backup - #87

Open
Abhinavpv28 wants to merge 66 commits into
developfrom
feature/usblogupload_backup
Open

Feature/usblogupload backup#87
Abhinavpv28 wants to merge 66 commits into
developfrom
feature/usblogupload_backup

Conversation

@Abhinavpv28

Copy link
Copy Markdown
Contributor

No description provided.

Copilot AI review requested due to automatic review settings March 2, 2026 03:36
@Abhinavpv28
Abhinavpv28 requested a review from a team as a code owner March 2, 2026 03:36
@rdkcmf-jenkins

Copy link
Copy Markdown
Contributor

b'## Copyright scan failure
Commit: f159ad9
Report detail: https://gist.github.com/rdkcmf-jenkins/543dfb4ac77caf5939e26ed8e35bda47'

@rdkcmf-jenkins

Copy link
Copy Markdown
Contributor

b'## Blackduck scan failure details

Summary: 0 violations, 0 files pending approval, 2 files pending identification.

  • Protex Server Path: /home/blackduck/github/dcm-agent/87/rdkcentral/dcm-agent

  • Commit: f159ad9

Report detail: gist'

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR expands the usbLogUpload unit test suite and its build/run plumbing, adding new Google Test binaries for utility and archive modules and integrating them into the repository’s unit test runner.

Changes:

  • Added new GTest executables for usb_log_utils and usb_log_archive, and updated existing usbLogUpload tests.
  • Updated usbLogUpload/unittest autotools files (configure.ac, Makefile.am) to build/link the new test binaries.
  • Updated unit_test.sh to build and execute the new usbLogUpload unit tests and to enable coverage by default.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 14 comments.

Show a summary per file
File Description
usbLogUpload/unittest/usb_log_validation_gtest.cpp Refactors validation tests; replaces a device-compatibility test with a placeholder.
usbLogUpload/unittest/usb_log_utils_gtest.cpp Adds new unit tests for usb_log_utils.c with stubs for external dependencies.
usbLogUpload/unittest/usb_log_main_gtest.cpp Adds mocks include and custom main() plus cleanup for global mocks.
usbLogUpload/unittest/usb_log_file_manager_gtest.cpp Adjusts file-manager tests, adds a local remove_directory() stub, and adds custom main().
usbLogUpload/unittest/usb_log_archive_gtest.cpp Adds new unit tests for usb_log_archive.c with dependency stubs.
usbLogUpload/unittest/configure.ac Adds a new autotools configure.ac for the usbLogUpload unit test subproject.
usbLogUpload/unittest/Makefile.am Adds new test binaries and updates link inputs/libs.
usbLogUpload/src/usb_log_main.c Wraps the production main() in #ifndef GTEST_ENABLE to allow test builds.
usbLogUpload/include/usb_log_validation.h Removes validate_system_prerequisites() declaration from the public header.
unit_test.sh Enables coverage by default; builds and runs the new usbLogUpload test binaries; sets runtime env.
Comments suppressed due to low confidence (1)

unit_test.sh:25

  • ENABLE_COV is now set to true by default, which makes the --enable-cov flag effectively redundant and forces coverage tooling on every run. If coverage is meant to be optional, restore the default to false and only enable it when --enable-cov is provided.
ENABLE_COV=true

if [ "x$1" = "x--enable-cov" ]; then
      echo "Enabling coverage options"
      export CXXFLAGS="-g -O0 -fprofile-arcs -ftest-coverage"

Comment thread unit_test.sh
export top_srcdir=`pwd`

export LD_LIBRARY_PATH="/usr/local/lib:$TOP_DIR/uploadstblogs/src/.libs:$LD_LIBRARY_PATH"
echo "RDK_PROFILE=TV" >> /etc/device.properties

Copilot AI Mar 2, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unit_test.sh appends RDK_PROFILE=TV to /etc/device.properties unconditionally. This can create duplicate entries on repeated runs and makes test setup non-idempotent. Follow the pattern used elsewhere in the repo (grep before append, and create the file if missing) to keep the environment stable.

Suggested change
echo "RDK_PROFILE=TV" >> /etc/device.properties
if [ ! -f /etc/device.properties ]; then
touch /etc/device.properties
fi
if ! grep -q '^RDK_PROFILE=TV$' /etc/device.properties 2>/dev/null; then
echo "RDK_PROFILE=TV" >> /etc/device.properties
fi

Copilot uses AI. Check for mistakes.
Comment on lines +8 to +13

// Mocks and stubs for dependencies
extern "C" {
int get_current_timestamp(char *buf, size_t len) {
strncpy(buf, "01/01/26-12:00:00", len-1);
buf[len-1] = '\0';

Copilot AI Mar 2, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file uses strncpy in the stubbed get_current_timestamp() but does not include <cstring>/<string.h>. Some toolchains will fail to compile due to a missing declaration. Add the appropriate header (and consider using std::strncpy when including <cstring>).

Suggested change
// Mocks and stubs for dependencies
extern "C" {
int get_current_timestamp(char *buf, size_t len) {
strncpy(buf, "01/01/26-12:00:00", len-1);
buf[len-1] = '\0';
#include <cstring>
// Mocks and stubs for dependencies
extern "C" {
int get_current_timestamp(char *buf, size_t len) {
std::strncpy(buf, "01/01/26-12:00:00", len - 1);
buf[len - 1] = '\0';

Copilot uses AI. Check for mistakes.
Comment thread unit_test.sh
Comment on lines +46 to +48
cd ../..
sh cov_build.sh
cd -

Copilot AI Mar 2, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cov_build.sh is invoked unconditionally. This script clones/builds dependencies and adds network + time cost to every unit test run, even when coverage isn’t requested. Consider gating this behind the coverage flag (or a separate explicit option) and/or caching dependencies in CI.

Copilot uses AI. Check for mistakes.
Comment on lines 234 to 236
/**
* @brief Test temporary directory creation with NULL buffer
*/

Copilot AI Mar 2, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no longer a success-path test for create_temporary_directory() (the previous success test was removed), leaving only error-path coverage below. Add back a positive test that verifies the function returns 0, produces a valid path, and that the directory is created and cleaned up.

Copilot generated this review using guidance from repository custom instructions.
Comment on lines 55 to 57
TEST_F(UsbLogValidationTest, DeviceCompatibilityInvalidTest) {
// TODO: Test validate_device_compatibility with non-PLATCO device
// This would require mocking environment variables or config
EXPECT_TRUE(true); // Placeholder
}

/**
* @brief Test USB mount point validation with valid path
*/
TEST_F(UsbLogValidationTest, UsbMountPointValidTest) {
// TODO: Test validate_usb_mount_point with valid path
const char* valid_path = "/tmp";
EXPECT_EQ(validate_usb_mount_point(valid_path), 0);
EXPECT_TRUE(true);
}

Copilot AI Mar 2, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DeviceCompatibilityInvalidTest is currently a placeholder (EXPECT_TRUE(true)) and no longer exercises validate_device_compatibility(). This reduces coverage of a key gate in usb_log_main.c. Consider stubbing/mocking getDevicePropertyData("RDK_PROFILE", ...) to return a non-"TV" value and asserting the expected non-zero return code.

Copilot generated this review using guidance from repository custom instructions.
Comment on lines 71 to +74
char* test_argv[] = {(char*)"usblogupload", (char*)"/tmp/test_usb"};
// This would require refactoring main to be testable
EXPECT_TRUE(true); // Placeholder
} No newline at end of file
}

Copilot AI Mar 2, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MainArgumentValidationTest is still a placeholder, and the file also contains tests that call usb_log_upload_execute() without preparing the required filesystem state (mount point directory must exist, log paths must be set up, etc.). As written, the suite is likely to be flaky/fail on a clean CI runner. Either create/cleanup the required directories in the fixture or mock the filesystem + validation dependencies so assertions are deterministic.

Copilot uses AI. Check for mistakes.
Comment on lines +53 to +57
bool remove_directory(const char* dirpath) {
if (!dirpath) return false;
struct stat st;
// Return false if directory does not exist
if (stat(dirpath, &st) != 0 || !S_ISDIR(st.st_mode)) return false;

Copilot AI Mar 2, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test-local remove_directory() stub returns false when the directory does not exist, which differs from the production remove_directory() used by cleanup_temporary_files() (it treats “already removed” as success). This makes the unit tests validate behavior that the shipped code does not have. Align the stub semantics with production or explicitly mock remove_directory via the shared mock layer.

Copilot uses AI. Check for mistakes.
Comment on lines 93 to 97
// Directory should not exist yet
EXPECT_FALSE(access(usb_log_dir.c_str(), F_OK) == 0);

// Create directory should succeed
EXPECT_EQ(create_usb_log_directory(usb_log_dir.c_str()), 0);

Copilot AI Mar 2, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CreateUsbLogDirectorySuccessTest no longer verifies that the directory was actually created, and CleanupTemporaryFilesSuccessTest no longer verifies the directory was removed. With the current stubs, these tests can pass even if nothing is created/cleaned up. Add assertions on filesystem state (or use mocks that simulate state changes) so the tests validate observable behavior.

Copilot generated this review using guidance from repository custom instructions.
TEST(UsbLogUtilsTest, CopyFileAndDelete_Success) {
const char* src = "test_src.txt";
const char* dst = "test_dst.txt";
FILE* f = fopen(src, "w");

Copilot AI Mar 2, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CopyFileAndDelete_Success calls fputs() on f without checking whether fopen() succeeded. If the working directory is not writable, this will crash the test instead of failing cleanly. Add an ASSERT_NE(f, nullptr) (or equivalent) before writing.

Suggested change
FILE* f = fopen(src, "w");
FILE* f = fopen(src, "w");
ASSERT_NE(f, nullptr);

Copilot uses AI. Check for mistakes.
Comment thread unit_test.sh

cd ../uploadstblogs/unittest
cd ../..
sh cov_build.sh

Copilot AI Mar 2, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new call to sh cov_build.sh introduces a supply-chain risk because cov_build.sh clones and executes remote GitHub repositories (e.g., rdkcentral/iarmmgrs, rdkcentral/rdk_logger, rdkcentral/telemetry, rdkcentral/common_utilities) using mutable branches without any integrity or version pinning. If any of those upstream repositories or the network path is compromised, an attacker could execute arbitrary code in your CI/test environment and potentially access secrets or tamper with build artifacts. To mitigate this, pin git clone operations to specific commit hashes or verified release tags and/or vendor the required build artifacts instead of executing unpinned remote code during the test run.

Copilot uses AI. Check for mistakes.
@mhughesacn

Copy link
Copy Markdown

Hi @Abhinavpv28 : New code files should have standard Comcast headers please.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants